feat: enable null reference type support #2146

MaggieKimani1 · 2025-02-13T12:04:35Z

This PR:

Adds null reference type support by enabling the nullable feature in the core and readers project
Annotates all properties and method parameters/return types with ? for nullable types
Fixes compiler warnings such as possible dereferences of null values.

src/Microsoft.OpenApi/Extensions/EnumExtensions.cs

src/Microsoft.OpenApi/Reader/ParsingContext.cs

@@ -274,7 +274,7 @@
            if ((version.is2_0() || version.is3_0()) && (doc.Paths == null))
            {
                // paths is a required field in OpenAPI 2.0 and 3.0 but optional in 3.1
-                RootNode.Context.Diagnostic.Errors.Add(new OpenApiError("", $"Paths is a REQUIRED field at {RootNode.Context.GetLocation()}"));
+                RootNode?.Context.Diagnostic.Errors.Add(new OpenApiError("", $"Paths is a REQUIRED field at {RootNode.Context.GetLocation()}"));


To fix the problem, we need to ensure that RootNode is not null before accessing its Context property. The best way to do this is to add a null check for RootNode before the line where it is dereferenced. If RootNode is null, we can either log an error or handle it appropriately to avoid the NullReferenceException.

src/Microsoft.OpenApi/Reader/V31/OpenApiV31Deserializer.cs

src/Microsoft.OpenApi/Services/OpenApiWorkspace.cs

+                }
+                else if (_artifactsRegistry.TryGetValue(uri, out var artifact))
+                {
+                    return (T)(object)artifact;


src/Microsoft.OpenApi/Validations/Rules/OpenApiSchemaRules.cs

+                    if ((!childItem.Properties?.ContainsKey(discriminatorName) ?? false) &&
+                                        (!childItem.Required?.Contains(discriminatorName) ?? false))
+                    {
+                        return ValidateChildSchemaAgainstDiscriminator(childItem, discriminatorName);
+                    }
+                    else
+                    {
+                        return true;
+                    }


src/Microsoft.OpenApi/Writers/OpenApiYamlWriter.cs

baywet

Great work here!
I've only done a partial review at this point starting with the interfaces, since changes there will require reviewing implementations again.

baywet · 2025-02-14T13:07:59Z

src/Microsoft.OpenApi/Interfaces/IOpenApiVersionService.cs

@@ -19,7 +19,7 @@ internal interface IOpenApiVersionService
        /// <param name="summary">The summary of the reference.</param>
        /// <param name="description">A reference description</param>
        /// <returns>The <see cref="OpenApiReference"/> object or null.</returns>
-        OpenApiReference ConvertToOpenApiReference(string reference, ReferenceType? type, string summary = null, string description = null);
+        OpenApiReference? ConvertToOpenApiReference(string reference, ReferenceType? type, string? summary = null, string? description = null);


why do we accept a nullable reference type here? shouldn't it be required?

It can be null in the case of an externally referenced file:

OpenAPI.NET/src/Microsoft.OpenApi/Reader/V2/OpenApiV2VersionService.cs

Lines 142 to 151 in 85c3a4e

// Either this is an external reference as an entire file

// or a simple string-style reference for tag and security scheme.

if (type == null)

{

// "$ref": "Pet.json"

return new()

{

ExternalResource = segments[0]

};

}

I'm not sure I understand this case? "it's something from another file but we don't know what it is" ??? how is that possible since the reference itself will be in a "known section" (i.e. the reference will be where a schema is, etc...)

src/Microsoft.OpenApi/Interfaces/IOpenApiReferenceHolder.cs

baywet · 2025-02-14T13:16:30Z

src/Microsoft.OpenApi/Models/OpenApiExtensibleDictionary.cs

@@ -27,16 +27,16 @@ protected OpenApiExtensibleDictionary():this(null) { }
        /// <param name="dictionary">The generic dictionary.</param>
        /// <param name="extensions">The dictionary of <see cref="IOpenApiExtension"/>.</param>
        protected OpenApiExtensibleDictionary(


wouldn't it be better to make the dictionary parameter non nullable and optional with a default value of [] (if that's possible) ?

Yeah that's possible. However I didn't want to tamper with/change model data as it might have side effects during serialization

src/Microsoft.OpenApi/Models/OpenApiOperation.cs

baywet · 2025-02-14T13:22:23Z

src/Microsoft.OpenApi/Models/OpenApiReference.cs


        /// <summary>
        /// External resource in the reference.
        /// It maybe:
        /// 1. a absolute/relative file path, for example:  ../commons/pet.json
        /// 2. a Url, for example: http://localhost/pet.json
        /// </summary>
-        public string ExternalResource { get; init; }
-
+        public string? ExternalResource { get; init; }


leaving the comment here because I can't below.
I'm not sure Type should be nullable

src/Microsoft.OpenApi/Models/OpenApiRequestBody.cs

src/Microsoft.OpenApi/Models/OpenApiResponse.cs

baywet · 2025-02-14T13:27:19Z

src/Microsoft.OpenApi/Models/OpenApiResponse.cs

@@ -136,7 +140,7 @@ public void SerializeAsV2(IOpenApiWriter writer)

                        foreach (var example in Content
                            .Where(mediaTypePair => mediaTypePair.Value.Examples != null && mediaTypePair.Value.Examples.Any())
-                            .SelectMany(mediaTypePair => mediaTypePair.Value.Examples))
+                            .SelectMany(mediaTypePair => mediaTypePair.Value.Examples!))


don't use bang operators , they usually lead to bugs.
Add an OfType() after the select many instead, not only it'll filter the null values at runtime, it'll fix the compiler complaining about nullable values.

Using OfType() still doesn't work. There's still a possible null reference return at the SelectMany():

foreach (var example in Content .Where(mediaTypePair => mediaTypePair.Value.Examples is not null && mediaTypePair.Value.Examples.Any()) .SelectMany(mediaTypePair => mediaTypePair.Value.Examples) .OfType<KeyValuePair<string, IOpenApiExample>>()) { writer.WritePropertyName(example.Key); example.Value.SerializeAsV2(writer); }

Co-authored-by: Vincent Biret <[email protected]>

sonarqubecloud · 2025-02-17T08:38:18Z

Quality Gate failed

Failed conditions
56.6% Coverage on New Code (required ≥ 80%)
30.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Co-authored-by: Vincent Biret <[email protected]>

MaggieKimani1 added 3 commits February 13, 2025 14:42

feat: enable NRT

56bb532

fix: dereference of a possible null reference

b3b5663

Merge branch 'main' into feat/enable-NRT

c99dfc3

github-advanced-security bot found potential problems Feb 13, 2025

View reviewed changes

MaggieKimani1 added 2 commits February 14, 2025 15:47

chore: code cleanup

489fa4d

chore: convert to conditional expression

0b6b07d

MaggieKimani1 changed the title ~~feat: enable NRT~~ feat: enable null reference type support Feb 14, 2025

MaggieKimani1 marked this pull request as ready for review February 14, 2025 13:04

MaggieKimani1 requested review from irvinesunday, darrelmiller, gavinbarron, millicentachieng, andrueastman and baywet as code owners February 14, 2025 13:04

baywet requested changes Feb 14, 2025

View reviewed changes

Update src/Microsoft.OpenApi/Models/OpenApiOperation.cs

f13a9e4

Co-authored-by: Vincent Biret <[email protected]>

baywet mentioned this pull request Feb 20, 2025

adds support for OpenAPI 3.1 descriptions microsoft/kiota#5936

Open

MaggieKimani1 and others added 3 commits February 21, 2025 11:07

Update src/Microsoft.OpenApi/Models/OpenApiRequestBody.cs

b7b845c

Co-authored-by: Vincent Biret <[email protected]>

chore: PR feedback

ea5c389

Merge branch 'main' into feat/enable-NRT

78b8d88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable null reference type support #2146

feat: enable null reference type support #2146

MaggieKimani1 commented Feb 13, 2025 •

edited

Loading

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

baywet left a comment

baywet Feb 14, 2025

MaggieKimani1 Feb 21, 2025

baywet Feb 21, 2025

baywet Feb 14, 2025

MaggieKimani1 Feb 20, 2025

baywet Feb 14, 2025

baywet Feb 14, 2025

MaggieKimani1 Feb 21, 2025

sonarqubecloud bot commented Feb 17, 2025

	// Either this is an external reference as an entire file
	// or a simple string-style reference for tag and security scheme.
	if (type == null)
	{
	// "$ref": "Pet.json"
	return new()
	{
	ExternalResource = segments[0]
	};
	}

feat: enable null reference type support #2146

Are you sure you want to change the base?

feat: enable null reference type support #2146

Conversation

MaggieKimani1 commented Feb 13, 2025 • edited Loading

baywet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 17, 2025

Quality Gate failed

MaggieKimani1 commented Feb 13, 2025 •

edited

Loading